@tobixen Is this better now? I’ll pull it tomorrow and work on CI errors. I committed the changes as suggestions for now.

🤖 This comment is claude-generated.

Code Review: feature/jmap

Summary

Well-structured implementation. Clean layered design, passes ruff lint/format, 254 unit tests all green. A few things worth addressing before merging.

Architecture

caldav/jmap/
├── __init__.py          get_jmap_client / get_async_jmap_client factory
├── client.py            JMAPClient (sync)
├── async_client.py      AsyncJMAPClient (mirrors all public methods)
├── session.py           Session establishment + account discovery
├── error.py             Exception hierarchy
├── constants.py         Capability URNs
├── objects/             Dataclasses: JMAPCalendar, JMAPEvent, JMAPTask, JMAPTaskList
├── methods/             Pure functions: request builders + response parsers
└── convert/             iCalendar ↔ JSCalendar bidirectional conversion

Public API matches the CalDAV client pattern: get_calendars, create_event, get_event, update_event, delete_event, search_events, get_sync_token, get_objects_by_sync_token, task CRUD. Both sync and async variants.

Issues

Medium — potential crash in create_event (client.py:257):

if "new-0" in not_created:
    self._raise_set_error(session, not_created["new-0"])
return created["new-0"]["id"]   # KeyError if server omits it from both dicts

If the server returns a malformed response where "new-0" is absent from both created and not_created, you get an uncaught KeyError. Low probability but worth hardening — guard with if "new-0" not in created: or wrap in try/except KeyError.

Medium — non-deterministic participant UUIDs (ical_to_jscal.py):

Every conversion generates fresh uuid.uuid4() for participant IDs. If you fetch a JMAP event, convert to iCal, edit, and push back, participant IDs change on every call. Use uuid.uuid5(uuid.NAMESPACE_URL, email) to make them deterministic and stable across round-trips.

Medium — no SSL verification option (client.py):

DAVClient has ssl_verify_cert=False for testing against self-signed certs (Zimbra, local servers). JMAPClient has no equivalent — all requests use default requests.post() settings. Given the project's test infrastructure this will be needed.

Low — get_objects_by_sync_token makes two HTTP round-trips (client.py:410–436):

Issues CalendarEvent/changes then a separate CalendarEvent/get. search_events correctly batches query+get with a result reference in one HTTP call. The same pattern could apply here when hasMoreChanges is false.

Low — RELATED=END alarm trigger not handled (jscal_to_ical.py):

VALARM triggers with TRIGGER;RELATED=END:-PT15M (relative to event end) are silently treated as start-relative. The RELATED parameter is not checked.

Low — incomplete generic type hints on objects:

Dict fields in objects/event.py, objects/task.py etc. are typed as bare dict rather than dict[str, bool] or dict[str, dict]. Not a runtime bug, but reduces IDE support.

Low — async client has zero integration test coverage:

test_jmap_integration.py only exercises JMAPClient. AsyncJMAPClient mirrors all methods but has never been run against a real server. A smoke test (get_calendars) would at least catch obvious wiring issues.

Documentation

docs/source/jmap.rst (377 lines) is comprehensive — covers quick start, auth modes, CRUD, search, sync tokens, task API, and async usage. Good.

CHANGELOG entry has been added in the branch.

Thanks for the review! Responses inline:

potential crash in create_event (client.py:257) — KeyError if server omits "new-0" from both dicts

Valid, will fix.

non-deterministic participant UUIDs — use uuid.uuid5 for stable IDs across round-trips

False positive. The UUID is a local ephemeral key within a single CalendarEvent/set call, the server assigns its own stable participant IDs. Stability across round-trips is a server responsibility in JMAP.

no SSL verification option

Noted, out of scope for this PR.

get_objects_by_sync_token makes two HTTP round-trips

Intentional. CalendarEvent/changes returns separate created and updated lists, and we need to preserve that distinction in the return value. A result reference would merge them, losing the added/modified split.

RELATED=END alarm trigger not handled

Valid, will fix.

bare dict type hints

Noted, low priority.

async client has zero integration test coverage

TestAsyncJMAPEventIntegration in test_jmap_integration.py covers create, get, update, delete, search, sync token, and incremental sync against live Cyrus, 6 tests total.

Sorry for not getting back with human-generated comments yet. My 15yo son was dragging me for a monster skiing trip yesterday, we managed to get home only after midnight, quite exhausted. Will have a calmer shorter skiing trip with my daughter today before getting back to this.